Skip to content

gridlook-jupyter: server extension + S3 range proxy (phase 4 of issue #1)#5

Merged
espg merged 8 commits into
mainfrom
claude/1-phase4-gridlook-jupyter
Jul 20, 2026
Merged

gridlook-jupyter: server extension + S3 range proxy (phase 4 of issue #1)#5
espg merged 8 commits into
mainfrom
claude/1-phase4-gridlook-jupyter

Conversation

@espg

@espg espg commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Refs #1 (phase 4)

gridlook-jupyter: an in-fork jupyter/ package shipping one jupyter-server extension that serves the built Vite app and a streaming S3 byte-range proxy, per the ratified phase 4 plan on #1.

Ratified leans → what landed

Lean Landed as
(1) SPA data addressing: paste-the-proxy-URL, plus the s3:// rewrite as a same-PR follow-up commit if trivial Both. Pasting /gridlook/s3/<bucket>/<key> works with zero frontend involvement; the last commit adds the rewrite: an s3://bucket/prefix data input rewrites to <app-base>/s3/bucket/prefix only when the app detects it is extension-served (one cached probe of api/health, with a body check so SPA-fallback hosts that answer 200 to any path don't false-positive). Standalone serving leaves input untouched.
(2) Streaming proxy, firmly S3ProxyHandler streams obstore chunks with per-chunk flush — whole objects are never buffered, no presigned URLs, credentials stay hub-side (ambient chain). GET/HEAD only, Range pass-through with 206 + Content-Range, no LIST.
(3) Build-hook packaging hatch_build.py runs npm ci && npm run build at the repo root and copies dist/ into the wheel as gridlook_jupyter/static/ (gitignored; re-included via hatchling artifacts). Honest failure modes: empty static + missing frontend sources, or no npm on PATH → the build errors with a pointed message; it never ships an empty wheel, and CI asserts static/index.html is present in the built wheel. Editable installs skip the hook — GridlookProxy.static_dir points at a dev dist/. Node requirement documented in jupyter/README.md.
(4) Allowlist-only auth Empty allowlist (the default) = proxy disabled: every proxy request 403s with a message saying so and naming the config knob. Non-allowlisted bucket → 403 naming the bucket. Config via traitlets (GridlookProxy.allowed_buckets, .region) with env fallback (GRIDLOOK_ALLOWED_BUCKETS, GRIDLOOK_S3_REGION); a configured traitlet wins over env.

Also per plan: route family /gridlook/ (static SPA via AuthenticatedFileHandler) + /gridlook/api/health + /gridlook/s3/<bucket>/<key>; the /gridlook/hive/ namespace is reserved with a routing comment pointing at the phase-6 moczarr virtual-store endpoint — not implemented here. v1 launcher is the documented URL (launcher card is phase 5). pip install auto-enables the extension via jupyter-config/jupyter_server_config.d shipped as wheel shared-data. The README names the cryocloud/2i2c deployment target (englacial/zagg#301 context).

Store-factory seam

GridlookProxy.store_factory is a plain attribute (default builds an obstore S3Store per allowlisted bucket, cached). Tests point it at LocalStore(prefix=tmp/<bucket>) and exercise the whole proxy surface with zero real S3.

Phases

  • Server extension: static SPA + health + streaming S3 proxy, config surface, pytest suite
  • Wheel build hook packaging the Vite dist
  • CI: build the wheel + run the Python gates on PRs touching jupyter/ or src/
  • s3:// → proxy rewrite in the SPA data input (follow-up commit per lean (1))

How tested

  • Python: 27 tests green (pytest -v, pytest-jupyter server fixtures) — extension load/health, static index + asset serving, bare-route 301, proxy 200 full GET, 206 with exact byte slices verified (bounded, open-ended, and suffix ranges), HEAD, content-type passthrough/guess, 404 missing key, 403 non-allowlisted (bucket named), 403 disabled-when-empty-allowlist, allowlist via traitlets and via env, and a range-header parser matrix. ruff check + ruff format --check clean (line-length 100, E/F/W/I/N in jupyter/pyproject.toml).
  • Frontend: npm run test (79 tests incl. 12 new rewrite tests covering served/standalone/probe-failure/fallback-host/caching), typecheck, lint-ci, build all green.
  • Wheel: uv build --wheel jupyter from the repo checkout runs the npm hook and produces a wheel containing the SPA plus the etc/jupyter auto-enable config (verified by listing the wheel); the no-npm failure path was also exercised and errors with the pointed message.

Questions for review

  • Redirect-follow in tests: /gridlook (no trailing slash) 301s to /gridlook/. The test asserts the 301 + Location without following it — following redirects in tornado's test client requires a DNS lookup that sandboxed dev environments can block (the follow works fine in real deployments and CI).
  • Multi-range requests: the proxy handles single ranges only; multi-range Range headers are ignored and the full object is served with 200 (RFC 9110 permits ignoring Range; zarrita only issues single ranges). Flag if you'd rather see 416.
  • Per-user scoping (your note on the plan thread): not in this PR per "starts opt-in later" — the allowlist is global to the server. The natural seam for per-user checks is S3ProxyHandler._store_for, where self.current_user is available.
  • Commit type for CI: commitlint's type-enum here has no ci type, so the workflow commit is chore(config).

🤖 Generated with Claude Code

https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax

result = await obstore.get_async(store, key, options=options)
except (NotFoundError, FileNotFoundError) as e:
raise web.HTTPError(404, f"no such object: s3://{bucket}/{key}") from e
except (BaseError, OSError) as e:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Malformed keys raise an uncaught ValueError → HTTP 500 (should be 400). obstore's path parser rejects keys containing .. or empty segments with a plain builtins.ValueError (Could not parse path), which is neither NotFoundError/FileNotFoundError nor BaseError/OSError, so it escapes both except clauses here (and the identical block in head()). Probed against the live proxy with the test fixtures:

s3/test-bucket/../secret/passwd.bin  -> 500
s3/test-bucket/data/../secret/x      -> 500
s3/test-bucket/....//x               -> 500

Good news first: this is not a traversal breach — obstore rejects the path before any I/O, so nothing escapes the bucket prefix, and your write_error override suppresses the traceback body (client sees only error 500). But the status is wrong (a malformed client request is a 400, not a 500) and each hit logs a full traceback server-side. No test covers a malformed key. Suggest catching ValueError here and in head()HTTPError(400, ...).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 16796da: both get() and head() now catch obstore's ValueError and return 400 invalid key: <key> (no traceback logged). Added parametrized GET+HEAD tests for ../secret/x, a/../b, ....//x asserting 400 (no 500). Also gitignored the stray jupyter/uv.lock in the same commit.

raise web.HTTPError(404, f"no such object: s3://{bucket}/{key}") from e
except (BaseError, OSError) as e:
# Includes out-of-bounds ranges; obstore surfaces them as generic errors.
raise web.HTTPError(502, f"S3 error for s3://{bucket}/{key}: {e}") from e

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

Unsatisfiable ranges surface as 502 instead of 416. An out-of-bounds range is raised by obstore as GenericError (a BaseError subclass, "Requested range was invalid"), so it is caught here and mapped to 502 Bad Gateway — mislabeling a client range error as an upstream/gateway fault. RFC 9110 §15.5.17 calls for 416 Range Not Satisfiable. Probed against the live proxy (object is 1024 B):

Range: bytes=999999-1000000  -> 502

Related parser edge: Range: bytes=-0 yields {"suffix": 0} → a 206 with a malformed Content-Range: bytes 1024-1023/1024 (last-byte-pos < first-byte-pos) and a 0-byte body. Both are arguably 416. zarrita only issues bounded in-range requests, so this is low severity, but a client probing EOF gets a confusing 502 rather than a clean 416. Consider mapping the "range invalid" error → 416 and treating bytes=-0 as unsatisfiable.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 2b7a4ae: an out-of-range obstore error on a range request now returns 416 with Content-Range: bytes */<size>, and bytes=-0 is rejected as unsatisfiable (416) at parse time. On the size choice: I emit the numeric bytes */<size> (one HEAD on this cold error path), not bytes */* — RFC 9110 §14.4 requires a numeric complete-length in the unsatisfied-range form, so */* is only a fallback if that HEAD itself fails. Added tests: OOB and bytes=-0 → 416 with bytes */1024; existing 206 byte-exact tests stay green.

Comment thread jupyter/hatch_build.py Outdated
raise RuntimeError(f"gridlook-jupyter: npm run build produced no {dist}/index.html")
if static.exists():
shutil.rmtree(static)
shutil.copytree(dist, static)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

The wheel ships JS source maps. vite.config.ts sets build.sourcemap: true, so npm run build emits dist/assets/*.js.map, and this copytree(dist, static) copies them wholesale into gridlook_jupyter/static/ → they ride along in the distributed pip wheel. That is avoidable bloat (maps are often comparable in size to the bundles themselves) in a server-extension artifact that never needs them, and the CI unzip -l | grep index.html assertion would not notice them. Low severity — the frontend is open source, so this is not a secret disclosure — but worth stripping the .map files after copy (or building the wheel with sourcemaps off) so the wheel stays lean.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in e0231b5: the build hook now copies diststatic with ignore_patterns("*.map"), so the wheel no longer ships sourcemaps (vite.config.ts is untouched — dev/standalone builds keep their maps). Added a CI step in jupyter.yml asserting ! unzip -l *.whl | grep .map$. Verified locally: fresh uv build --wheel jupyter builds clean, wheel contains static/index.html and zero .map files.

espg and others added 3 commits July 19, 2026 14:42
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
@espg
espg marked this pull request as ready for review July 19, 2026 21:47
@espg
espg merged commit d94f216 into main Jul 20, 2026
3 checks passed
@espg
espg deleted the claude/1-phase4-gridlook-jupyter branch July 20, 2026 05:32
espg added a commit that referenced this pull request Jul 21, 2026
espg added a commit that referenced this pull request Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant